@ArielElp made 3 comments.
Reviewable status: 7 of 12 files reviewed, 3 unresolved discussions (waiting on @nimrod-starkware and @yoavGrs).

crates/starknet_committer/src/db/facts_db/types.rs line 56 at r3 (raw file):

Done. (the constants, not separate function)

crates/starknet_committer/src/patricia_merkle_tree/leaf/leaf_serde.rs line 55 at r3 (raw file):

I don't follow, we want this method ignored, we have a separate DBObject implementation.

Unrelated to this PR, it may have made things slightly more clear if we separate the "base" leaf types from the facts layout db types (who happen to be the same), but not worth it IMO.

crates/starknet_patricia_storage/src/db_object.rs line 52 at r3 (raw file):

Done. (different name)

@yoavGrs reviewed 5 files and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArielElp).

crates/starknet_committer/src/patricia_merkle_tree/leaf/leaf_serde.rs line 55 at r3 (raw file):

I suggested creating a base leaf.
Anyway, that's not going to happen anytime soon.

@dorimedini-starkware reviewed 12 files and all commit messages, and made 4 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ArielElp and @yoavGrs).

crates/starknet_committer/src/patricia_merkle_tree/leaf/leaf_serde.rs line 55 at r3 (raw file):

index layout wraps leaves, but facts layout uses the "base" leaves?
i.e. index layout wraps fact-y leaves and changes behavior?
a bit confusing, no?
suggestion (not in this PR!):

1. the DBObject get_db_key trait function definition should include a separator argument
2. seems like it can have a default implementation after (1), right? no need to impl 3 times here
3. separator should be defined per layout (const method? associated const, if such a thing exists?)

crates/starknet_patricia_storage/src/db_object.rs line 52 at r3 (raw file):

why do you have a specific implementation (implementation that doesn't fit all trait implementors) on the trait itself?
I would remove the default impl here, and reinstate it once the separator is a trait-associated-value (or something equivalent) of DBObject trait

crates/starknet_patricia/src/patricia_merkle_tree/traversal_test.rs line 52 at r5 (raw file):

    ) -> DbKey {
        // Dummy key for testing purposes (we only need a key when interacting with storage).
        DbKey(TEST_PREFIX.to_vec())

no longer equivalent, right? previously the DBKey had an extra ":"
doesn't matter though, better this way

Code quote:

DbKey(TEST_PREFIX.to_vec())

crates/starknet_committer/src/db/index_db/types.rs line 184 at r5 (raw file):

        let prefix = L::get_static_prefix(key_context);
        let suffix = self.root_index.0.to_be_bytes();
        create_db_key(prefix, &[], &suffix)

1. make it explicit
2. we may want to change this in the future, so no hard-coded literals

const INDEX_DB_KEY_SEPARATOR: &[u8] = b"";

Suggestion:

create_db_key(prefix, INDEX_DB_KEY_SEPARATOR, &suffix)

@ArielElp made 3 comments.
Reviewable status: 1 of 12 files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware, @nimrod-starkware, and @yoavGrs).

crates/starknet_committer/src/db/index_db/types.rs line 184 at r5 (raw file):

Done (there's already EMPTY_DB_KEY_SEPARATOR, made it pub)

crates/starknet_committer/src/patricia_merkle_tree/leaf/leaf_serde.rs line 55 at r3 (raw file):

Agree on it being a bit confusing. We can have separate wrappers at the end of the stack, though it's unrelated to the question ATM.

Re 1-3, an associated const on DBObject itself seems like the way to go. DBObject impl is clearly layout-dependent, and that way we don't worry about propogating it from e.g. Layout all the way to create_db_key.

TLDR of the changes now:

1. Still two arguments
2. We do have a default impl now that fits everyone
3. Associated const on DBObject

crates/starknet_patricia_storage/src/db_object.rs line 52 at r3 (raw file):

Irrelevant given the discussion above.

@dorimedini-starkware reviewed 11 files and all commit messages, made 1 comment, and resolved 3 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArielElp).

crates/starknet_committer/src/db/index_db/types.rs line 187 at r6 (raw file):

        let prefix = L::get_static_prefix(key_context);
        let suffix = self.root_index.0.to_be_bytes();
        create_db_key(prefix, INDEX_LAYOUT_DB_KEY_SEPARATOR, &suffix)

maybe this is better... separator should really be part of DBObjects only, WDYT?

Suggestion:

IndexFilledNode::<L>::DB_KEY_SEPARATOR

@dorimedini-starkware reviewed 1 file and all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArielElp).

@dorimedini-starkware reviewed 2 files and all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArielElp).

@dorimedini-starkware reviewed 2 files and all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArielElp).